Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Typo fix in DataTransformers.ipynb #176

Merged
merged 6 commits into from
Jan 27, 2021
Merged

Typo fix in DataTransformers.ipynb #176

merged 6 commits into from
Jan 27, 2021

Conversation

data-hound
Copy link
Contributor

Under Section 2, Multiple Inputs

class MultiOutputTransformer(BaseEstimator, TransformerMixin):
    ...

class MultiOutputClassifier(KerasClassifier):

    @property
    def feature_encoder(self):
        return MultiInputTransformer(...)

changed to

class MultiInputTransformer(BaseEstimator, TransformerMixin):
    ...


class MultiInputClassifier(KerasClassifier):

    @property
    def feature_encoder(self):
        return MultiInputTransformer(...)

@adriangb
Copy link
Owner

adriangb commented Jan 23, 2021 •
edited
Loading

Thank you for the bug report! I will review your PR shortly.

@codecov-io
Copy link

codecov-io commented Jan 23, 2021 •
edited
Loading

Codecov Report

Merging #176 (3da1bc3) into master (7fa51a4) will decrease coverage by 0.45%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
- Coverage   99.55%   99.09%   -0.46%     
==========================================
  Files           6        5       -1     
  Lines         679      666      -13     
==========================================
- Hits          676      660      -16     
- Misses          3        6       +3
Impacted Files Coverage Δ
scikeras/_saving_utils.py 96.20% <0.00%> (-3.80%) ⬇️
scikeras/wrappers.py 99.44% <0.00%> (-0.01%) ⬇️
scikeras/__init__.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fa51a4...3da1bc3. Read the comment docs.

adriangb
adriangb reviewed Jan 24, 2021
View reviewed changes
.pre-commit-config.yaml Outdated
@@ -1,12 +1,12 @@
repos:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the changes here intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. After I ran the pre-commit test, and everything was fine, I was asked to stage this file for commit as well, and so I did.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, not sure what happened there. Would you mind reverting this change then?

@adriangb
Copy link
Owner

I'm actually in the middle of a pretty large overhaul of the docs in #174, the main goal being to make it easier to edit notebooks. Would you mind waiting until I merge #174 and then we can use this change as a case study for the effectiveness of the new system?

@data-hound
Copy link
Contributor Author

I see the #174 has been merged. I will make the changes then.

@adriangb
Copy link
Owner

Awesome, thank you! Please let me know if you encounter any issues with the new setup.

@adriangb
Copy link
Owner

adriangb commented Jan 27, 2021 •
edited
Loading

Hey @data-hound it looks like the way I set up CI won't work for forks 🤦 . I think I'll be able to get it fixed, but it might take a couple of days. That is to say, don't worry about the CI failure, and sorry for wasting your time with broken CI. In the meantime, could you please revert the change to .pre-commit-config.yaml ? Thanks!

@adriangb adriangb mentioned this pull request Jan 27, 2021
Merged
@adriangb adriangb merged commit 52d99ac into adriangb:master Jan 27, 2021
@adriangb
Copy link
Owner

I was able to make the changes. Thank you very much @data-hound !

@data-hound
Copy link
Contributor Author

I was able to make the changes. Thank you very much @data-hound !

Mention Not!
Seems you did the rectification for the pre-commit-config.yaml in the branch as well - Thanks for that!

@data-hound data-hound deleted the tutorial_typos_fix branch January 29, 2021 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adriangb adriangb adriangb left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@data-hound @adriangb @codecov-io